Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to new MOI nonlinear interface #1052

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Conversation

blegat
Copy link
Contributor

@blegat blegat commented Oct 8, 2023

JuMP v1.15 now supports providing the nonlinear objective and constraints with @objective and @constraint.
The old syntax @NLobjective and @NLconstraint is kept for backward compatibility only.
Since the MOI wrapper in Optim is new, we don't need to support the old interface so this PR replace it the new one which works with @objective and @constraint.

@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

Merging #1052 (1d64631) into master (760993c) will decrease coverage by 0.32%.
The diff coverage is 71.05%.

❗ Current head 1d64631 differs from pull request most recent head e4e17d3. Consider uploading reports for the commit e4e17d3 to get more accurate results

@@            Coverage Diff             @@
##           master    #1052      +/-   ##
==========================================
- Coverage   84.94%   84.62%   -0.32%     
==========================================
  Files          44       44              
  Lines        3427     3422       -5     
==========================================
- Hits         2911     2896      -15     
- Misses        516      526      +10     
Files Coverage Δ
src/MOI_wrapper.jl 74.27% <71.05%> (-4.06%) ⬇️

... and 7 files with indirect coverage changes

@pkofod
Copy link
Member

pkofod commented Oct 9, 2023

lgtm

@blegat
Copy link
Contributor Author

blegat commented Oct 9, 2023

Should be ready from my side as well

@blegat
Copy link
Contributor Author

blegat commented Dec 4, 2023

@pkofod bump

@pkofod
Copy link
Member

pkofod commented Dec 12, 2023

I'll review it, thanks

@pkofod
Copy link
Member

pkofod commented Dec 12, 2023

Okay, fairly small change compared to the original. LGTM

@pkofod pkofod merged commit 75ff19c into JuliaNLSolvers:master Dec 12, 2023
6 checks passed
@odow
Copy link
Contributor

odow commented Dec 12, 2023

This PR did not appropriately update the compat in Project.toml: #1060

@ccoffrin
Copy link

ccoffrin commented Jan 1, 2024

@blegat, is there a way this can support both the old and the new interface?

@blegat
Copy link
Contributor Author

blegat commented Jan 2, 2024

It's not too hard, what's the motivation ?

@ccoffrin
Copy link

ccoffrin commented Jan 2, 2024

Legacy code may not yet be updated to the new NLP interface.

Now that I think of it, I think it would be ok if there was one version that worked with the old NLP interface and another version that only supported the new one.

When I tried to use the old NLP interface with the current tagged release of Optim it did not seem to work for me.

@blegat
Copy link
Contributor Author

blegat commented Jan 2, 2024

Yes, the addition of the old NLP was merged a few months ago so after the latest release. I thought that since it's a new feature there is no need to maintain support for the old interface

@ccoffrin
Copy link

ccoffrin commented Jan 2, 2024

Ya I agree no need to maintain support for the old interface, but can we make sure there is a release where the old interface works?

@blegat
Copy link
Contributor Author

blegat commented Jan 19, 2024

The issue is that v1 has already been tagged so if we release a version with the old interface and then remove it, it's a breaking change and we need v2

@ccoffrin
Copy link

Good point. I was very confused when I made my first comment on this. I thought that in a previously release this package had been working with the legacy NL interface, but that is not the case.

I 100% agree going for the new NL interface, folks should be targeting that for new code development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants